Skip to content

i#2297: AARCH64: Implement mbr & cbr instrumentation #7005

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
May 19, 2025

Conversation

jiegec
Copy link
Contributor

@jiegec jiegec commented Sep 25, 2024

Implement mbr & cbr instrumentation for AARCH64, previously only available for X86. For cbr instrumentation, there are three types of conditional branches on AARCH64:

  1. conditional branch if (non) zero: generate code to compare the register against zero to compute the direction
  2. test bit and branch if (non) zero: generate code to extract the bit out of the register (using ubfm) and then compare the bit against zero to compute the direction
  3. conditional branch according to NZCV: generate code to compute the direction using csinc instruction

Special care is implemented if the stolen register x28 is used as the operand, to avoid generating incorrect results.

For mbr instrumentation, the target address is placed in the register, which is later passed to the clean call from the user.

Additional instruction creation macros are added for ubfm and csinc instructions. Support for immediate operand is added to the INSTR_CREATE_eor macro. XINST_CREATE_slr_s is updated to use the new INSTR_CREATE_ubfm macro.

Count-ctis test is now enabled due to cbr & mbr instrumentation available on AARCH64. It has been enhanced to validate the direction and target address of conditional branches to validate the instrumentation.

The instrcalls and cbrtrace example clients are enabled as well. They are tested on real AARCH64 hardware. Additionally, a manually crafted dynamorio client can successfully capture the whole control flow transfer history with this pull reuqest.

Fixes: #2297 (only for AARCH64, ARM not yet) and #2919.

Copy link
Contributor

@derekbruening derekbruening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution. The main missing piece is testing: see the comments.

@jiegec
Copy link
Contributor Author

jiegec commented Oct 28, 2024

Help is wanted to fix mbr instrumentation: it sometimes receives small integers instead of actual target address, possibly some indices into the basic box cache?

@derekbruening
Copy link
Contributor

Help is wanted to fix mbr instrumentation: it sometimes receives small integers instead of actual target address, possibly some indices into the basic box cache?

I assume it has the wrong register or something. There are no such "indices" in in-fragment code. Use the logs https://dynamorio.org/page_logging.html to see where the app's register values goes and which one is passed to the clean call.

@jiegec jiegec force-pushed the i2297-cbr-aarch64 branch from f02728e to d9222e1 Compare May 10, 2025 07:33
@jiegec jiegec changed the title i#2297: AARCH64: Implement cbr instrumentation i#2297: AARCH64: Implement mbr & cbr instrumentation May 10, 2025
@jiegec
Copy link
Contributor Author

jiegec commented May 10, 2025

Newly enabled count-ctis test is passed. Manually written dynamorio client to leverage cbr/mbr instrumentation also works correctly.

@jiegec
Copy link
Contributor Author

jiegec commented May 10, 2025

A bug has been found: when x28 is used for cbnz conditional branch, the branch direction is wrongly computed. It is possibly due to x28 being used by dynamorio. dr_insert_get_stolen_reg_value() is required to fix this.

@jiegec
Copy link
Contributor Author

jiegec commented May 10, 2025

A bug has been found: when x28 is used for cbnz conditional branch, the branch direction is wrongly computed. It is possibly due to x28 being used by dynamorio. dr_insert_get_stolen_reg_value() is required to fix this.

Fixed in commit e9980ab. With this pull request, we are able to record the control flow transfer (regarding each branch) of a program on AARCH64 correctly.

@derekbruening
Copy link
Contributor

Please click the re-request review in the upper right when ready (comments resolved and tests green).

@jiegec jiegec requested a review from derekbruening May 13, 2025 05:53
@derekbruening
Copy link
Contributor

@jiegec jiegec force-pushed the i2297-cbr-aarch64 branch from f02728e to d9222e1
3 days ago

Please do not force-push shared branches. This destroys the history and makes it a lot harder for reviewers! This is documented in several places: https://dynamorio.org/page_contributing.html, https://dynamorio.org/page_code_reviews.html#autotoc_md118. There is now no way to easily see the changes since the last round of reviews. You're forcing the reviewer to start from scratch; throwing away the anchors to all the prior comments; etc.

@jiegec
Copy link
Contributor Author

jiegec commented May 14, 2025

@jiegec jiegec force-pushed the i2297-cbr-aarch64 branch from f02728e to d9222e1
3 days ago

Please do not force-push shared branches. This destroys the history and makes it a lot harder for reviewers! This is documented in several places: https://dynamorio.org/page_contributing.html, https://dynamorio.org/page_code_reviews.html#autotoc_md118. There is now no way to easily see the changes since the last round of reviews. You're forcing the reviewer to start from scratch; throwing away the anchors to all the prior comments; etc.

I'm sorry for that. It spans too long time for this pull request for me to remember the rules..

Copy link
Contributor

@derekbruening derekbruening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good if additional sanity test cases are added which should be straightforward.

@jiegec jiegec requested a review from derekbruening May 15, 2025 09:14
To test the new cbr instrumentation of AARCH64, add additional branch pc
check: dst == src + 0x8, and direction check to count-ctis. Region of
interest is marked via nop; nop; add; nop sequence. The triple nops
pattern does not work, because it also appears in other places.
@jiegec jiegec requested a review from derekbruening May 18, 2025 12:06
Copy link
Contributor

@derekbruening derekbruening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM: Thank you for improving the test, this gives more confidence the API works and gives us a good regression test to detect unforeseen issues with future code changes.

@derekbruening
Copy link
Contributor

I've sent you an invite to gain commit access. You can then merge here (need to resolve all the conversations by clicking Resolve first). You can then use branches on this DynamoRIO/dynamorio repository directly rather than a clone.

@jiegec jiegec merged commit 58cb327 into DynamoRIO:master May 19, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement dr_insert_cbr_instrumentation on ARM & AArch64
3 participants